fix: decouple Beholder observation metrics from Prometheus scrape cycle#21720
fix: decouple Beholder observation metrics from Prometheus scrape cycle#21720
Conversation
emate
requested review from
0xAustinWang,
KodeyThomas,
chris-de-leon-cll,
Copilot,
matYang,
mateusz-sekara,
tt-cll and
winder
|
👋 emate, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
There was a problem hiding this comment.
Pull request overview
Risk Rating: HIGH (adds a long-lived background goroutine that triggers metric collection concurrently with Prometheus scrapes; incorrect deltas or panics would directly impact telemetry reliability and could crash the process)
Decouples OCR3 observation metric forwarding to Beholder from the Prometheus scrape cycle by adding a periodic polling loop that calls Collect() on the wrapped counters on a fixed interval.
Changes:
- Start a background polling loop (default 10s) after
libocr3.NewOracle(...)so wrapped counters publish deltas even without/metricsscrapes. - Add
Start(interval)+poll()toObservationMetricsCollector, with cancellation viaClose(). - Add a unit test verifying polling publishes deltas without an external Prometheus scrape.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| core/capabilities/ccip/oraclecreator/plugin.go | Starts metrics collector polling after oracle creation and ensures it’s closed with the oracle. |
| core/capabilities/ccip/oraclecreator/observation_metrics_collector.go | Adds polling interval, lifecycle context/cancel, and the polling goroutine to drive delta publishing independently of scrapes. |
| core/capabilities/ccip/oraclecreator/observation_metrics_collector_test.go | Adds a test asserting polling publishes and does not republish without new increments. |
Scrupulous human review recommended (high-impact areas):
ObservationMetricsCollector.Start(...)/poll()interaction with Prometheus scrapes: this introduces concurrentCollect()calls and can cause incorrect delta publishing unless the wrapped-counter delta tracking is made concurrency-safe.- Lifecycle correctness: ensuring
Start()cannot panic (invalid interval) and thatClose()reliably stops any background goroutines in all runtime paths.
Reviewer recommendations (based on CODEOWNERS for /core/capabilities/ccip):
@smartcontractkit/ccip-offchain(primary owners for this directory)@smartcontractkit/keystoneand/or@smartcontractkit/capabilities-team(owners for/core/capabilities/broadly)
|
|
|
@emate question on the PR, do all Beholder metrics require this sort of polling to work, or is there something special with the implementation of the Beholder participation metric ? |
@anedos-chainlink This is specific to this implementation. Other Beholder metrics use the OTel push model. The ObservationMetricsCollector is a workaround for a specific constraint: libocr3 only exposes ocr3_sent_observations_total and ocr3_included_observations_total as Prometheus counters — there is no OTel/Beholder implementation in libocr3. Since we can't instrument libocr3 code directly, this collector wraps the Prometheus Collect() call to detect counter deltas and forward them to Beholder. The side effect is that the Beholder publication is scrape-driven (tied to /metrics scrape intervals) rather than event-driven. |
Thanks for the explanation 👍 |
| w.publisher.PublishMetric(context.Background(), w.metricName, delta, w.labels) | ||
| // CAS loop: ensures only one concurrent caller (background poll or Prometheus | ||
| // scrape) advances lastValueBits and publishes the delta for a given interval. | ||
| for { |
There was a problem hiding this comment.
This seems to be a really busy loop, it doesn't wait for a timer for example. Is this expected? Are we sure this won't have CPU usage effects?
There was a problem hiding this comment.
I actually rethought this loop and removed it completely as I removed publishing metrics on Prometheus Collect(), leaving to publish it only on a set interval.
| w.mu.Lock() | ||
| delta := metricValue - w.lastValue | ||
| if delta > 0 { | ||
| w.lastValue = metricValue | ||
| w.mu.Unlock() | ||
| w.logger.Debugw("Observation metric incremented", | ||
| "metric", w.metricName, | ||
| "value", metricValue, | ||
| "delta", delta, | ||
| "labels", w.labels, | ||
| ) | ||
| if w.publisher != nil { | ||
| w.publisher.PublishMetric(context.Background(), w.metricName, delta, w.labels) | ||
| } | ||
| } else { | ||
| w.mu.Unlock() |
There was a problem hiding this comment.
It seems like you can put the w.mu.Unlock() after the if/else?
w.mu.Lock()
delta := metricValue - w.lastValue
if delta > 0 {
w.lastValue = metricValue
w.logger.Debugw("Observation metric incremented",
"metric", w.metricName,
"value", metricValue,
"delta", delta,
"labels", w.labels,
)
if w.publisher != nil {
w.publisher.PublishMetric(context.Background(), w.metricName, delta, w.labels)
}
}
w.mu.Unlock()Or alternatively, wrap in a function and use defer (common pattern):
func() {
w.mu.Lock()
defer w.mu.Unlock()
delta := metricValue - w.lastValue
if delta > 0 { ... } // omitted for clarity
}() There was a problem hiding this comment.
I've removed the mutex completely since publishing now happens exclusively inside readAndPublish(), which is only ever called from the single ticker goroutine
| // Start the background polling loop after NewOracle so all counters are already registered. | ||
| i.metricsCollector.Start(defaultPollingInterval) |
There was a problem hiding this comment.
Is it expected that we will start the same metricsCollector for each oracle? We have a sync.Once so that seems to be OK, but I'm wondering if this should be global or not.
There was a problem hiding this comment.
Fixed it to be less confusing (see the comment below)
| // Add metrics collector to closers so it's properly shut down | ||
| closers = append(closers, i.metricsCollector) |
There was a problem hiding this comment.
I'm not sure this will yield the expected behavior, we could e.g. shut down a particular DON via a config change, and this will shut down the collector for everyone, no?
There was a problem hiding this comment.
yeah so this was a bit confusing because metricsCollector was only a temp value. I've moved it to be a return parameter from setupObservationMetricsCollector as it's much cleaner this way.
|
…le (#21720) * fix: decouple Beholder observation metrics from Prometheus scrape cycle * Address review comments * Add startOnce * Remove the unnecessary CAS loop * Fix tests & other refactor fixes * Fix * Review fixes
…le (#21720) * fix: decouple Beholder observation metrics from Prometheus scrape cycle * Address review comments * Add startOnce * Remove the unnecessary CAS loop * Fix tests & other refactor fixes * Fix * Review fixes
ocr3_sent_observations_total(andocr3_included_observations_total) were only forwarded to Beholder when Prometheus scraped the/metricsendpoint. ThewrappedCounter.Collect()method — the only place deltas are computed and PublishMetric is called — is driven entirely by the Prometheus scrape cycle.On nodes where scrapes were slow, missing, or misaligned, the Beholder counter would appear stuck even though the underlying OCR3 protocol was functioning normally.
Added a background polling loop to ObservationMetricsCollector that ticks every 10 seconds (matching the OTel PeriodicReader default) and calls poll(), which invokes Collect() on each wrapped counter directly